Skip to content

Conversation

@oleonardolima
Copy link
Contributor

  • Fixes the implementation of GetKey for KeyRequest::Bip32 when there's Xpriv has an origin, and it matches with the given KeyRequest::Bip32 derivation_path. It should strip the matching part, to correctly derive the key at the correct child number.
  • Updates the existing test to the correct and expected behavior.

- Fixes the implementation of `GetKey` for `KeyRequest::Bip32` when
  there's `Xpriv` has an origin, and it matches with the given
`KeyRequest::Bip32` derivation_path. It should strip the matching part,
to correctly derive the key at the correct child number.
- Updates the existing test to the correct and expected behavior.
@oleonardolima oleonardolima force-pushed the fix/getkey-bip32-key-origin-deriv branch from b8f4d24 to 2f72b27 Compare October 10, 2025 01:23
if let Some(matched_path) = descriptor_xkey.matches(key_source, secp) {
let (_, full_path) = key_source;

let derivation_path = &full_path[matched_path.len()..];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matched path includes the xkey.derivation_path, therefore we need to derive the xkey.derivation_path together with the remaining un-matched path. That's my hunch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never resolved, did you investigate this @oleonardolima?

Is this what you were talking about at the Summit @ValuedMammal or was that a different bug you think you found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, VM explained this issue to me at the summit.

I'm planning on working on this issue and adding more integration tests for different signing scenarios this week.

@ValuedMammal let me know if you found any other issues with it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will work, following the same implementation in bdk.

if let Some(..) = descriptor_xkey.matches(key_source, secp) {
    let (_, full_path) = key_source;

    let derived = match &descriptor_xkey.origin {
        Some((_, origin_path)) => {
            let derivation_path = &full_path[origin_path.len()..];
            descriptor_xkey.xkey.derive_priv(secp, &derivation_path)?
        }
        None => descriptor_xkey.xkey.derive_priv(secp, &full_path)?,
    };

    return Ok(Some(derived.to_priv()));
}

@apoelstra
Copy link
Member

Yep, I think this is correct.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2f72b27; successfully ran local tests

@apoelstra apoelstra merged commit 1179ea2 into rust-bitcoin:master Oct 10, 2025
31 checks passed
@oleonardolima oleonardolima deleted the fix/getkey-bip32-key-origin-deriv branch October 13, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants